Add regression test for multiple engine reloads #704
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I also removed a few members that I discovered were unused in my debugging. |
mjcarroll
changed the title
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
mjcarroll
force-pushed
the
mjcarroll/ogre_depth_camera_segfault
branch
from
f604212 to
88421a4
Compare
Signed-off-by: Michael Carroll <[email protected]>
|
I have been unable to reproduce |
|
@darksylinc There seems to be something specifically wrong with the OGRE 1 depth camera. I have valgrind/gdb for a bit and I'm kind of stumped. I think it's a viewport not getting disconnected correctly? If you have a quick guess I can try to track down, it would be appreciated. Valgrind seems a bit more interesting: |
|
Actually, this is a better valgrind trace (without some of my modifications/checks) |
|
Didn't I submit a PR to fix similar issues recently? I can't find it! |
|
Ahhh it wasn't a PR It was a patch: #679 (comment) I suspect the other problems have similar solutions. |
|
To be more specific this part was super important: if (this->dataPtr->thermalInstance)
{
// Do not leave a reference to this->dataPtr->thermalMaterial
Ogre::MaterialPtr nullMaterial;
this->dataPtr->thermalInstance->getTechnique()
->getOutputTargetPass()
->getPass(0)
->setMaterial(nullMaterial);
}
Because otherwise a texture would linger in a material that still exists. We need to unset the texture from that material. That material was used by the compositor. |
|
To repro all this I need to checkout https://github.com/gazebosim/gz-rendering/tree/mjcarroll/ogre_depth_camera_segfault ? |
That is all that is necessary. I attempted to duplicate the patch from the thermal and wide angle cameras, but this one doesn't use the same structure to the texture/material/compositor |
|
Valgrind is indeed finding something wrong on Gazebo side.
This stray |
|
This patch fixes INTEGRATION_depth_camera: diff --git a/ogre/src/OgreDepthCamera.cc b/ogre/src/OgreDepthCamera.cc
index 0dd31cce..b6139696 100644
--- a/ogre/src/OgreDepthCamera.cc
+++ b/ogre/src/OgreDepthCamera.cc
@@ -109,12 +109,14 @@ void OgreDepthCamera::Destroy()
if (this->dataPtr->pcdTexture)
{
- this->dataPtr->pcdTexture->RenderTarget()->removeAllViewports();
+ this->dataPtr->pcdTexture->Destroy();
+ this->dataPtr->pcdTexture.reset();
}
if (this->dataPtr->colorTexture)
{
- this->dataPtr->colorTexture->RenderTarget()->removeAllViewports();
+ this->dataPtr->colorTexture->Destroy();
+ this->dataPtr->colorTexture.reset();
}
if (!this->ogreCamera || !this->scene->IsInitialized())
diff --git a/ogre/src/OgreRenderTarget.cc b/ogre/src/OgreRenderTarget.cc
index d7ed64af..c1f02a81 100644
--- a/ogre/src/OgreRenderTarget.cc
+++ b/ogre/src/OgreRenderTarget.cc
@@ -63,8 +63,7 @@ OgreRenderTarget::~OgreRenderTarget()
{
// TODO(anyone): clean up check null
- OgreRTShaderSystem::Instance()->DetachViewport(this->ogreViewport,
- this->scene);
+ GZ_ASSERT(this->ogreViewport == nullptr, "DestroyTarget not called!");
}
//////////////////////////////////////////////////
@@ -339,6 +338,8 @@ void OgreRenderTexture::DestroyTarget()
if (nullptr == this->ogreTexture)
return;
+ this->materialApplicator.reset();
+
OgreRTShaderSystem::Instance()->DetachViewport(this->ogreViewport,
this->scene);
@@ -351,6 +352,7 @@ void OgreRenderTexture::DestroyTarget()
auto engine = OgreRenderEngine::Instance();
engine->OgreRoot()->getRenderSystem()->_cleanupDepthBuffers(false);
+ this->ogreViewport = nullptr;
this->ogreTexture = nullptr;
}
Edit: The patches causes other tests to fail because they're not calling renderTarget->Destroy(); |
|
OK here goes another attempt. Nothing crashes anymore, INTEGRATION_gpu_rays_ogre_gl3plus and INTEGRATION_lidar_visual_ogre_gl3plus fail; but they don't crash: diff --git a/ogre/src/OgreCamera.cc b/ogre/src/OgreCamera.cc
index fafc217a..76ec9469 100644
--- a/ogre/src/OgreCamera.cc
+++ b/ogre/src/OgreCamera.cc
@@ -44,6 +44,12 @@ void OgreCamera::Destroy()
if (!this->ogreCamera)
return;
+ if (this->renderTexture)
+ {
+ this->renderTexture->Destroy();
+ this->renderTexture = nullptr;
+ }
+
Ogre::SceneManager *ogreSceneManager;
ogreSceneManager = this->scene->OgreSceneManager();
if (ogreSceneManager == nullptr)
diff --git a/ogre/src/OgreDepthCamera.cc b/ogre/src/OgreDepthCamera.cc
index 0dd31cce..b6139696 100644
--- a/ogre/src/OgreDepthCamera.cc
+++ b/ogre/src/OgreDepthCamera.cc
@@ -109,12 +109,14 @@ void OgreDepthCamera::Destroy()
if (this->dataPtr->pcdTexture)
{
- this->dataPtr->pcdTexture->RenderTarget()->removeAllViewports();
+ this->dataPtr->pcdTexture->Destroy();
+ this->dataPtr->pcdTexture.reset();
}
if (this->dataPtr->colorTexture)
{
- this->dataPtr->colorTexture->RenderTarget()->removeAllViewports();
+ this->dataPtr->colorTexture->Destroy();
+ this->dataPtr->colorTexture.reset();
}
if (!this->ogreCamera || !this->scene->IsInitialized())
diff --git a/ogre/src/OgreRenderTarget.cc b/ogre/src/OgreRenderTarget.cc
index d7ed64af..a170e52a 100644
--- a/ogre/src/OgreRenderTarget.cc
+++ b/ogre/src/OgreRenderTarget.cc
@@ -63,8 +63,7 @@ OgreRenderTarget::~OgreRenderTarget()
{
// TODO(anyone): clean up check null
- OgreRTShaderSystem::Instance()->DetachViewport(this->ogreViewport,
- this->scene);
+ GZ_ASSERT(this->ogreViewport == nullptr, "Destroy() not called!");
}
//////////////////////////////////////////////////
@@ -339,6 +338,8 @@ void OgreRenderTexture::DestroyTarget()
if (nullptr == this->ogreTexture)
return;
+ this->materialApplicator.reset();
+
OgreRTShaderSystem::Instance()->DetachViewport(this->ogreViewport,
this->scene);
@@ -351,6 +352,7 @@ void OgreRenderTexture::DestroyTarget()
auto engine = OgreRenderEngine::Instance();
engine->OgreRoot()->getRenderSystem()->_cleanupDepthBuffers(false);
+ this->ogreViewport = nullptr;
this->ogreTexture = nullptr;
}
diff --git a/ogre/src/OgreThermalCamera.cc b/ogre/src/OgreThermalCamera.cc
index 508c267c..19be0544 100644
--- a/ogre/src/OgreThermalCamera.cc
+++ b/ogre/src/OgreThermalCamera.cc
@@ -291,6 +291,12 @@ void OgreThermalCamera::Destroy()
this->dataPtr->thermalImage = nullptr;
}
+ if (this->dataPtr->thermalTexture)
+ {
+ this->dataPtr->thermalTexture->Destroy();
+ this->dataPtr->thermalTexture = nullptr;
+ }
+
if (!this->ogreCamera || !this->scene->IsInitialized())
return;
diff --git a/ogre/src/OgreWideAngleCamera.cc b/ogre/src/OgreWideAngleCamera.cc
index 860dd2c1..856e244e 100644
--- a/ogre/src/OgreWideAngleCamera.cc
+++ b/ogre/src/OgreWideAngleCamera.cc
@@ -156,6 +156,12 @@ void OgreWideAngleCamera::Destroy()
this->dataPtr->wideAngleImage = nullptr;
}
+ if (this->dataPtr->wideAngleTexture)
+ {
+ this->dataPtr->wideAngleTexture->Destroy();
+ this->dataPtr->wideAngleTexture = nullptr;
+ }
+
for (unsigned int i = 0u; i < this->dataPtr->kEnvCameraCount; ++i)
{
if (this->dataPtr->envRenderTargets[i]) |
Co-authored-by: Matias N. Goldberg <[email protected]> Signed-off-by: Michael Carroll <[email protected]>
Codecov Report
@@ Coverage Diff @@
## gz-rendering7 #704 +/- ##
================================================
Coverage ? 74.18%
================================================
Files ? 159
Lines ? 14370
Branches ? 0
================================================
Hits ? 10661
Misses ? 3709
Partials ? 0 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Signed-off-by: Michael Carroll <[email protected]>
mjcarroll
force-pushed
the
mjcarroll/ogre_depth_camera_segfault
branch
from
8425e98 to
845480d
Compare
iche033
approved these changes
Aug 16, 2022
| @@ -0,0 +1,241 @@ | |||
| /* | |||
| * Copyright (C) 2017 Open Source Robotics Foundation | |||
|
@osrf-jenkins retest this please |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🦟 Bug fix
Summary
Similar to previous crashes we have had, we weren't appropriately cleaning up resources in our rendering objects.
I uncovered this while testing
gz-sensors7, as it wasn't revealed by tests here.As a result, I have added a regression test that will attempt to load and unload a rendering engine multiple times while also instantiating sensors. If everything is cleaned up correctly, this test runs without any segfaults.
A have also included fixes for the DepthCamera and GpuRays sensor that were detected via this test.
Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-bymessages.